Initial POC commit for adding ReadOnlyMemory overloads to StringSet o…#2221
Initial POC commit for adding ReadOnlyMemory overloads to StringSet o…#2221Stabzs wants to merge 1 commit intoStackExchange:mainfrom
Conversation
| /// <remarks> | ||
| /// <seealso href="https://redis.io/commands/mset"/>, | ||
| /// <seealso href="https://redis.io/commands/msetnx"/> | ||
| /// </remarks> |
There was a problem hiding this comment.
possibly worth a warning about FireAndForget being dangerous here if the ReadOnlyMemory is going to be recycled/reused?
There was a problem hiding this comment.
Definitely seems wise, thank you. I'll add warnings for all of the new methods taking ReadOnlyMemory. If the approach looks good overall, I'll continue expanding the surface area.
|
|
||
| for (int i = 0; i < outer.Length; ++i) | ||
| { | ||
| inner[i] = ToInner(outer.Span[i]); |
There was a problem hiding this comment.
Accessing .Span is relatively expensive; this should be done outside the loop and resused.
There was a problem hiding this comment.
The ++i is very unusual for C#, note
There was a problem hiding this comment.
Makes sense, thanks! I'll make the changes to cache all call sites for .Span.
The ++i was copied from line 801 of the diff. I assumed that was intentional and copied it here. Is that an incorrect usage?
|
Not wrong, just atypical. Consistency/copy-paste is a valid reason here!
…On Sat, 27 Aug 2022, 00:09 Stabzs, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/StackExchange.Redis/KeyspaceIsolation/WrapperBase.cs
<#2221 (comment)>
:
> @@ -804,6 +807,25 @@ internal WrapperBase(TInner inner, byte[] keyPrefix)
}
}
+ protected ReadOnlyMemory<KeyValuePair<RedisKey, RedisValue>> ToInner(ReadOnlyMemory<KeyValuePair<RedisKey, RedisValue>> outer)
+ {
+ if (outer.Length == 0)
+ {
+ return outer;
+ }
+ else
+ {
+ KeyValuePair<RedisKey, RedisValue>[] inner = new KeyValuePair<RedisKey, RedisValue>[outer.Length];
+
+ for (int i = 0; i < outer.Length; ++i)
+ {
+ inner[i] = ToInner(outer.Span[i]);
I'll make the changes to cache all call sites for .Span.
The ++i was copied from line 801 of the diff. I assumed that was
intentional and copied it here. Is that an incorrect usage?
—
Reply to this email directly, view it on GitHub
<#2221 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEHMDLPK2KLL5VTQUUT43V3FFC7ANCNFSM566WTIXA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
3 participants
…perations.